-
-
Notifications
You must be signed in to change notification settings - Fork 833
hide empty roomsublists when filtering via search/tagpanel #1954
Conversation
@@ -347,8 +347,14 @@ var RoomSubList = React.createClass({ | |||
var label = this.props.collapsed ? null : this.props.label; | |||
|
|||
let content; | |||
if (this.state.sortedList.length === 0 && !this.props.searchFilter && this.props.extraTiles.length === 0) { | |||
content = this.props.emptyContent; | |||
if (this.state.sortedList.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not include && this.props.extraTiles.length === 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup you're right, mine worked by an assumption that the current uses of <RoomSubList />
are the only possible use cases but in future, we may have alternate ones.
if (this.state.sortedList.length === 0 && !this.props.searchFilter && this.props.extraTiles.length === 0) { | ||
content = this.props.emptyContent; | ||
if (this.state.sortedList.length === 0) { | ||
// check if emptyContent is defined, if falsey then don't show an empty sublist even without searchFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, this comment makes things more confusing. I would at least write it in the order of conditions, i.e. "If no search filter is applied and ..." or "Only show placeholder if ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done thanks
// case insensitive if room name includes filter, | ||
// or if starts with `#` and one of room's aliases starts with filter | ||
return list.filter((room) => (room.name && room.name.toLowerCase().includes(lcFilter)) || | ||
(filter[0] === '#' && room.getAliases().some((alias) => alias.toLowerCase().startsWith(lcFilter)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like a merge commit, it includes changes to include matches against room aliases.
That's fine, but let's squash this PR (when merging) for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it would be clearer to separate the delint from the actual changes. @t3chguy, is there any chance you could squash the actual change commits into a single commit? That should leave us with a single delint commit and a single commit for actual changes.
Sorry for faff.
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
2fd5833
to
fabdf22
Compare
restaged things for you @lukebarnard1 |
Fixes element-hq/element-web#6801